Add template time helpers#180
Conversation
Reviewer's GuideThis PR adds deterministic time primitives ( Class diagram for new time helper types (TimestampValue and TimeDeltaValue)classDiagram
class TimestampValue {
+OffsetDateTime datetime
+iso8601() String
}
class TimeDeltaValue {
+Duration duration
+iso8601() String
}
TimestampValue --|> Object
TimeDeltaValue --|> Object
Object <|.. TimestampValue
Object <|.. TimeDeltaValue
TimestampValue : +get_value(key: Value) Option<Value>
TimeDeltaValue : +get_value(key: Value) Option<Value>
Class diagram for new time helper registration in stdlibclassDiagram
class Environment {
+add_function(name: String, func: Fn)
}
class TimeHelpers {
+register_functions(env: Environment)
+now(kwargs: Kwargs) Result<Value, Error>
+timedelta(kwargs: Kwargs) Result<Value, Error>
}
Environment <|.. TimeHelpers
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughAdd a runtime dependency on Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Template
participant Env as Environment
participant Time as Stdlib::time
participant Tc as time crate
Note over Template,Env: Render evaluates template expression using stdlib time helpers
Template->>Env: eval "{{ now(offset='+02:30').iso8601 }}"
Env->>Time: now(offset="+02:30")
Time->>Tc: OffsetDateTime::now_utc()
Time->>Tc: parse offset "+02:30"
Tc-->>Time: UtcOffset(+02:30)
Time-->>Env: TimestampValue{iso8601, unix_timestamp, offset}
Env-->>Template: "2025-10-05T12:34:56+02:30"
rect rgb(230,245,255)
Note right of Time: New stdlib helper registration and ISO8601 formatting logic
end
sequenceDiagram
autonumber
actor Template
participant Env as Environment
participant Time as Stdlib::time
Template->>Env: eval "{{ timedelta(days=1, minutes=90).iso8601 }}"
Env->>Time: timedelta({days:1, minutes:90})
Time->>Time: accumulate components -> Duration
Time-->>Env: TimeDeltaValue{iso8601, seconds, nanoseconds}
Env-->>Template: "P1DT1H30M"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/stdlib/time/tests.rs:63` </location>
<code_context>
+#[rstest]
+fn now_rejects_invalid_offset() {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for additional invalid offset formats.
Please include cases like missing sign, out-of-range values (e.g., '+25:00'), malformed strings (e.g., '+01:60', '+01:01:61'), and empty strings to improve coverage of invalid offsets.
</issue_to_address>
### Comment 2
<location> `src/stdlib/time/tests.rs:97` </location>
<code_context>
+#[rstest]
+fn timedelta_supports_negative_values() {
</code_context>
<issue_to_address>
**suggestion (testing):** Test negative values for all timedelta components.
Please add tests for negative days, weeks, minutes, milliseconds, microseconds, and nanoseconds to ensure all components are covered.
</issue_to_address>
### Comment 3
<location> `src/stdlib/time/tests.rs:120` </location>
<code_context>
+#[rstest]
+fn timestamp_iso8601_property() {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for ISO8601 formatting with offsets and fractional seconds.
Please include test cases for timestamps with non-UTC offsets and fractional seconds to fully validate ISO8601 formatting.
</issue_to_address>
### Comment 4
<location> `src/stdlib/time/tests.rs:132` </location>
<code_context>
+#[rstest]
+fn timedelta_iso8601_property() {
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for ISO8601 formatting of negative and zero durations.
Please include test cases for negative and zero durations to ensure correct ISO8601 formatting behavior.
</issue_to_address>
### Comment 5
<location> `src/stdlib/time/mod.rs:68` </location>
<code_context>
+ Ok(Value::from_object(TimeDeltaValue::new(total)))
+}
+
+fn parse_offset(raw: &str) -> Result<UtcOffset, Error> {
+ let trimmed = raw.trim();
+ if trimmed.eq_ignore_ascii_case("z") {
</code_context>
<issue_to_address>
**issue (complexity):** Consider using time crate's built-in parsers/formatters and merging the two add helpers into one generic function to simplify the code.
```markdown
I think we can shave off quite a bit of custom logic by leaning on ‑time-’s own parsers/formatters and merging the two “add_…” helpers into one. For example:
1) Replace your manual `parse_offset` with a single call to `UtcOffset::parse` + a small `format_description`.
```rust
use time::format_description::FormatItem;
use time::macros::format_description;
static OFFSET_FMT: &[FormatItem<'_>] = format_description!(
// +HH:MM or -HH:MM or +HH:MM:SS (seconds optional)
"[offset_sign][offset_hour]:[offset_minute][optional: [:offset_second]]"
);
fn parse_offset(raw: &str) -> Result<UtcOffset, Error> {
UtcOffset::parse(raw.trim(), OFFSET_FMT)
.map_err(|_| invalid_offset(raw))
}
```
2) Collapse `add_seconds_component`/`add_nanoseconds_component` into one generic helper:
```rust
fn add_component(
mut total: Duration,
amount: Option<i64>,
mul: i64,
mk: fn(i64) -> Duration,
label: &str,
) -> Result<Duration, Error> {
if let Some(v) = amount {
let v = v.checked_mul(mul).ok_or_else(|| overflow_error(label))?;
total = total
.checked_add(mk(v))
.ok_or_else(|| overflow_error(label))?;
}
Ok(total)
}
// usage in `timedelta`:
total = add_component(total, weeks, SECONDS_PER_WEEK, Duration::seconds, "weeks")?;
total = add_component(total, nanosec, NANOS_PER_MILLISECOND, Duration::nanoseconds, "milliseconds")?;
```
This removes ~60 lines of duplication, keeps all your overflow/error‐handling, and pushes complex string‐massaging into `time` itself.
</issue_to_address>
### Comment 6
<location> `docs/netsuke-design.md:937` </location>
<code_context>
+The `now()` helper produces an object that renders as an ISO 8601
+timestamp and exposes `iso8601`, `unix_timestamp`, and `offset` accessors so
+templates can serialise or compare values without string parsing. It defaults
+to UTC but accepts an `offset="+HH:MM"` keyword argument that re-bases the
+captured time on another fixed offset. Time is captured lazily when the helper
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The word 'serialise' should use en-oxendic spelling: 'serialize'.
Please change 'serialise' to 'serialize' to conform to en-oxendic spelling conventions.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Use en-oxendic (-ize / -yse / -our) spelling and grammar.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +68 to +125 fn parse_offset(raw: &str) -> Result<UtcOffset, Error> {
let trimmed = raw.trim();
if trimmed.eq_ignore_ascii_case("z") {
return Ok(UtcOffset::UTC);
}
let (sign, rest) = if let Some(remaining) = trimmed.strip_prefix('+') {
(1_i64, remaining)
} else if let Some(remaining) = trimmed.strip_prefix('-') {
(-1_i64, remaining)
} else {
return Err(invalid_offset(raw));
};
let (hours_part, remaining) = rest.split_once(':').ok_or_else(|| invalid_offset(raw))?;
if hours_part.contains(':') {
return Err(invalid_offset(raw));
}
let (minutes_part, seconds_part) = match remaining.split_once(':') {
Some((mins, secs)) if !secs.contains(':') => (mins, Some(secs)),
Some(_) => return Err(invalid_offset(raw)),
None => (remaining, None),
};
if minutes_part.contains(':') {
return Err(invalid_offset(raw));
}
let hours = parse_component(hours_part, raw)?;
let minutes = parse_component(minutes_part, raw)?;
let seconds = seconds_part
.map(|value| parse_component(value, raw))
.transpose()?;
if !(0..=23).contains(&hours) || !(0..=59).contains(&minutes) {
return Err(invalid_offset(raw));
}
let seconds_value = seconds.unwrap_or_default();
if !(0..=59).contains(&seconds_value) {
return Err(invalid_offset(raw));
}
let total_seconds = sign
* (i64::from(hours) * SECONDS_PER_HOUR
+ i64::from(minutes) * SECONDS_PER_MINUTE
+ i64::from(seconds_value));
let total_seconds = i32::try_from(total_seconds).map_err(|_| invalid_offset(raw))?;
UtcOffset::from_whole_seconds(total_seconds).map_err(|err| {
Error::new(
ErrorKind::InvalidOperation,
format!("now offset '{raw}' is invalid: {err}"),
)
})
}❌ New issue: Complex Method |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +217 to +269 fn format_duration_iso8601(duration: Duration) -> String {
if duration.is_zero() {
return "PT0S".to_owned();
}
let mut buffer = String::new();
if duration.is_negative() {
buffer.push('-');
}
buffer.push('P');
let absolute = duration.abs();
let days = absolute.whole_days();
let mut remainder = absolute - Duration::days(days);
if days != 0 {
buffer.push_str(&days.to_string());
buffer.push('D');
}
let mut time_section = String::new();
let hours = remainder.whole_hours();
if hours != 0 {
time_section.push_str(&hours.to_string());
time_section.push('H');
remainder -= Duration::hours(hours);
}
let minutes = remainder.whole_minutes();
if minutes != 0 {
time_section.push_str(&minutes.to_string());
time_section.push('M');
remainder -= Duration::minutes(minutes);
}
let seconds = remainder.whole_seconds();
let nanos = remainder.subsec_nanoseconds();
if seconds != 0 || nanos != 0 {
time_section.push_str(&format_seconds_with_fraction(seconds, nanos));
}
if time_section.is_empty() {
if buffer.ends_with('P') {
buffer.push_str("T0S");
}
} else {
buffer.push('T');
buffer.push_str(&time_section);
}
buffer
}❌ New issue: Complex Method |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +124 to +133 fn timedelta_iso8601_property() {
let duration = Duration::seconds(SECONDS_PER_DAY + 30) + Duration::nanoseconds(500_000_000);
let value = Value::from_object(TimeDeltaValue::new(duration));
let object = value.as_object().expect("object");
let iso = object
.get_value(&Value::from("iso8601"))
.expect("iso8601 attr")
.to_string();
assert_eq!(iso, "P1DT30.5S");
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- rely on time's format_description parser for offsets and share a generic component accumulator to remove timedelta duplication\n- broaden time helper coverage with invalid offset cases, negative components, and ISO-8601 assertions for offsets, fractions, and zero/negative durations\n- align documentation spelling with en-oxendic guidance
There was a problem hiding this comment.
Gates Failed
(1 file with Excess Number of Function Arguments)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| mod.rs | 1 advisory rule | 9.69 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +110 to +127 fn add_component(
mut total: Duration,
amount: Option<i64>,
multiplier: i64,
constructor: fn(i64) -> Duration,
label: &str,
) -> Result<Duration, Error> {
if let Some(value) = amount {
let scaled = value
.checked_mul(multiplier)
.ok_or_else(|| overflow_error(label))?;
let component = constructor(scaled);
total = total
.checked_add(component)
.ok_or_else(|| overflow_error(label))?;
}
Ok(total)
}❌ New issue: Excess Number of Function Arguments |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9454 @leynos, I'll suggest a refactoring that groups related parameters into a struct to reduce the argument count from 5 to 3. Suggested FixIntroduce a struct ComponentSpec {
multiplier: i64,
constructor: fn(i64) -> Duration,
label: &'static str,
}
fn add_component(
mut total: Duration,
amount: Option<i64>,
spec: ComponentSpec,
) -> Result<Duration, Error> {
if let Some(value) = amount {
let scaled = value
.checked_mul(spec.multiplier)
.ok_or_else(|| overflow_error(spec.label))?;
let component = (spec.constructor)(scaled);
total = total
.checked_add(component)
.ok_or_else(|| overflow_error(spec.label))?;
}
Ok(total)
}Update the call sites in the fn timedelta(kwargs: &Kwargs) -> Result<Value, Error> {
let weeks: Option<i64> = kwargs.get("weeks")?;
let days: Option<i64> = kwargs.get("days")?;
let hours: Option<i64> = kwargs.get("hours")?;
let minutes: Option<i64> = kwargs.get("minutes")?;
let seconds: Option<i64> = kwargs.get("seconds")?;
let milliseconds: Option<i64> = kwargs.get("milliseconds")?;
let microseconds: Option<i64> = kwargs.get("microseconds")?;
let nanoseconds: Option<i64> = kwargs.get("nanoseconds")?;
kwargs.assert_all_used()?;
let mut total = Duration::ZERO;
total = add_component(total, weeks, ComponentSpec {
multiplier: SECONDS_PER_WEEK,
constructor: Duration::seconds,
label: "weeks",
})?;
total = add_component(total, days, ComponentSpec {
multiplier: SECONDS_PER_DAY,
constructor: Duration::seconds,
label: "days",
})?;
total = add_component(total, hours, ComponentSpec {
multiplier: SECONDS_PER_HOUR,
constructor: Duration::seconds,
label: "hours",
})?;
total = add_component(total, minutes, ComponentSpec {
multiplier: SECONDS_PER_MINUTE,
constructor: Duration::seconds,
label: "minutes",
})?;
total = add_component(total, seconds, ComponentSpec {
multiplier: 1,
constructor: Duration::seconds,
label: "seconds",
})?;
total = add_component(total, milliseconds, ComponentSpec {
multiplier: NANOS_PER_MILLISECOND,
constructor: Duration::nanoseconds,
label: "milliseconds",
})?;
total = add_component(total, microseconds, ComponentSpec {
multiplier: NANOS_PER_MICROSECOND,
constructor: Duration::nanoseconds,
label: "microseconds",
})?;
total = add_component(total, nanoseconds, ComponentSpec {
multiplier: 1,
constructor: Duration::nanoseconds,
label: "nanoseconds",
})?;
Ok(Value::from_object(TimeDeltaValue::new(total)))
}This reduces the argument count from 5 to 3, well below the threshold of 4. AI Coding Agent Prompt |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +52 to +138 fn timedelta(kwargs: &Kwargs) -> Result<Value, Error> {
let weeks: Option<i64> = kwargs.get("weeks")?;
let days: Option<i64> = kwargs.get("days")?;
let hours: Option<i64> = kwargs.get("hours")?;
let minutes: Option<i64> = kwargs.get("minutes")?;
let seconds: Option<i64> = kwargs.get("seconds")?;
let milliseconds: Option<i64> = kwargs.get("milliseconds")?;
let microseconds: Option<i64> = kwargs.get("microseconds")?;
let nanoseconds: Option<i64> = kwargs.get("nanoseconds")?;
kwargs.assert_all_used()?;
let mut total = Duration::ZERO;
total = add_component(
total,
weeks,
ComponentSpec {
multiplier: SECONDS_PER_WEEK,
constructor: Duration::seconds,
label: "weeks",
},
)?;
total = add_component(
total,
days,
ComponentSpec {
multiplier: SECONDS_PER_DAY,
constructor: Duration::seconds,
label: "days",
},
)?;
total = add_component(
total,
hours,
ComponentSpec {
multiplier: SECONDS_PER_HOUR,
constructor: Duration::seconds,
label: "hours",
},
)?;
total = add_component(
total,
minutes,
ComponentSpec {
multiplier: SECONDS_PER_MINUTE,
constructor: Duration::seconds,
label: "minutes",
},
)?;
total = add_component(
total,
seconds,
ComponentSpec {
multiplier: 1,
constructor: Duration::seconds,
label: "seconds",
},
)?;
total = add_component(
total,
milliseconds,
ComponentSpec {
multiplier: NANOS_PER_MILLISECOND,
constructor: Duration::nanoseconds,
label: "milliseconds",
},
)?;
total = add_component(
total,
microseconds,
ComponentSpec {
multiplier: NANOS_PER_MICROSECOND,
constructor: Duration::nanoseconds,
label: "microseconds",
},
)?;
total = add_component(
total,
nanoseconds,
ComponentSpec {
multiplier: 1,
constructor: Duration::nanoseconds,
label: "nanoseconds",
},
)?;
Ok(Value::from_object(TimeDeltaValue::new(total)))
}❌ New issue: Large Method |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10425 @leynos, I'll suggest a refactoring that uses a data-driven approach to reduce the function from 85 lines to approximately 40 lines. Suggested FixReplace the repetitive const COMPONENT_SPECS: &[(&str, ComponentSpec)] = &[
(
"weeks",
ComponentSpec {
multiplier: SECONDS_PER_WEEK,
constructor: Duration::seconds,
label: "weeks",
},
),
(
"days",
ComponentSpec {
multiplier: SECONDS_PER_DAY,
constructor: Duration::seconds,
label: "days",
},
),
(
"hours",
ComponentSpec {
multiplier: SECONDS_PER_HOUR,
constructor: Duration::seconds,
label: "hours",
},
),
(
"minutes",
ComponentSpec {
multiplier: SECONDS_PER_MINUTE,
constructor: Duration::seconds,
label: "minutes",
},
),
(
"seconds",
ComponentSpec {
multiplier: 1,
constructor: Duration::seconds,
label: "seconds",
},
),
(
"milliseconds",
ComponentSpec {
multiplier: NANOS_PER_MILLISECOND,
constructor: Duration::nanoseconds,
label: "milliseconds",
},
),
(
"microseconds",
ComponentSpec {
multiplier: NANOS_PER_MICROSECOND,
constructor: Duration::nanoseconds,
label: "microseconds",
},
),
(
"nanoseconds",
ComponentSpec {
multiplier: 1,
constructor: Duration::nanoseconds,
label: "nanoseconds",
},
),
];
fn timedelta(kwargs: &Kwargs) -> Result<Value, Error> {
let mut total = Duration::ZERO;
for (name, spec) in COMPONENT_SPECS {
let amount: Option<i64> = kwargs.get(name)?;
total = add_component(total, amount, *spec)?;
}
kwargs.assert_all_used()?;
Ok(Value::from_object(TimeDeltaValue::new(total)))
}This reduces the function to approximately 10 lines (well below the 70-line threshold) whilst maintaining identical behaviour. AI Coding Agent Prompt |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/stdlib/time/mod.rs (1)
52-138: Consolidated timedelta logic with ComponentSpec + add_componentThis resolves the “excess arguments” smell and removes duplication. Overflow paths use checked_mul/checked_add with stable error messages.
As per coding guidelines
Also applies to: 163-178
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/stdlib/time/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Keep functions small with single responsibility; apply command-query separation
Function documentation must include clear examples; test documentation should omit redundant examples
No single Rust code file may exceed 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; code must be clean under clippy
Fix warnings emitted during tests in code, do not silence them
Extract helper functions when a function is too long; maintain separation of concerns and CQRS
Group many related parameters into well-named structs
Consider using Arc for large error values to reduce data returned
Every module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
When suppressing lints, scope narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint handling
When functions are unused for certain features, gate with #[cfg]/#[cfg_attr]
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use cap-std for filesystem operations; avoid std::fs directly
Use camino for path handling; avoid std::path::PathBuf directly
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use opaque eyre::Report only at the app boundary; do not expose in public APIs
Never export opaque error types from libraries; convert to domain...
Files:
src/stdlib/time/mod.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/stdlib/time/mod.rs
🔍 Remote MCP Ref
Summary of additional, review-relevant facts discovered
-
Dependency change: Cargo.toml adds runtime dependency "time" with features = ["formatting", "macros", "parsing", "serde"].
-
New stdlib wiring: src/stdlib/mod.rs now declares mod time; and register() calls time::register_functions(env) to expose the new helpers. (Check wrap into existing registration order to ensure no ordering regressions.)
-
Core implementation: src/stdlib/time/mod.rs implements now() and timedelta() helpers, timestamp and duration wrapper types, ISO‑8601 formatting/parsing, offset parsing, error handling, and attribute accessors (iso8601, unix_timestamp, offset, seconds, nanoseconds). This file is large and contains the functions flagged for complexity by CodeScene (parse_offset and format_duration_iso8601) and the add_component helper with many parameters — all referenced in the PR review notes. Focus review on correctness of:
- parse_offset parsing semantics, range checks, and error messages (must preserve existing InvalidOperation messages used by tests).
- format_duration_iso8601 correctness for edge cases (zero, negative durations, fractional seconds) and exact ISO‑8601 output.
- add_component call sites in timedelta to ensure overflow/negative handling matches PR tests.
-
Tests added:
- Unit tests in src/stdlib/time/tests.rs exercising now() offsets, invalid offsets, timedelta components (including negatives and overflow), and ISO‑8601 outputs. Use these to validate behavior if refactoring.
- Feature tests in tests/features/stdlib_time.feature cover template rendering scenarios and error messages (overflow, invalid offset). These are the behavioral contract the implementation must satisfy.
Notes for reviewers (concise)
- Any refactor must preserve exact ISO‑8601 string outputs and the exact InvalidOperation error messages asserted by the new tests/feature scenarios.
- Pay special attention to overflow detection and sign handling in timedelta and offset parsing because tests assert both numeric properties and string representations.
- CI/linters may fail on cyclomatic complexity; suggested helper extractions in PR comments target parse_offset, format_duration_iso8601, and add_component — verify tests pass after any such refactor.
Sources consulted
- Cargo.toml (branch codex/implement-time-helpers-in-template-library)
- src/stdlib/mod.rs (branch codex/implement-time-helpers-in-template-library)
- src/stdlib/time/mod.rs (branch codex/implement-time-helpers-in-template-library)
- src/stdlib/time/tests.rs (branch codex/implement-time-helpers-in-template-library)
- tests/features/stdlib_time.feature (branch codex/implement-time-helpers-in-template-library)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, x86_64)
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-14, macos-arm64, arm64)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64)
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64)
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (3)
src/stdlib/time/mod.rs (3)
1-7: Good module docs and purpose statementKeep this. It satisfies the “module must begin with a //! doc comment” rule and explains the utility clearly.
As per coding guidelines
33-37: Registration looks correctRegister both helpers via closures capturing Kwargs by value and forwarding by reference. No stray kwargs due to assert_all_used().
220-242: Nice split; complexity is back under controlThe extraction into format_time_components and finalize_duration_buffer reduces cyclomatic complexity while preserving edge cases (zero, negative, fractional seconds).
As per coding guidelines
Also applies to: 244-268, 270-281
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.toml(1 hunks)src/stdlib/time/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Keep functions small with single responsibility; apply command-query separation
Function documentation must include clear examples; test documentation should omit redundant examples
No single Rust code file may exceed 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; code must be clean under clippy
Fix warnings emitted during tests in code, do not silence them
Extract helper functions when a function is too long; maintain separation of concerns and CQRS
Group many related parameters into well-named structs
Consider using Arc for large error values to reduce data returned
Every module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
When suppressing lints, scope narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint handling
When functions are unused for certain features, gate with #[cfg]/#[cfg_attr]
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use cap-std for filesystem operations; avoid std::fs directly
Use camino for path handling; avoid std::path::PathBuf directly
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use opaque eyre::Report only at the app boundary; do not expose in public APIs
Never export opaque error types from libraries; convert to domain...
Files:
src/stdlib/time/mod.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/stdlib/time/mod.rs
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: Use explicit version ranges in Cargo.toml and keep dependencies up to date
All dependencies must use SemVer caret requirements (e.g., "1.2.3")
Forbid wildcard (*) and open-ended (>=) version specifiers; use ~ only with documented, specific reasons
Files:
Cargo.toml
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, x86_64)
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (7)
Cargo.toml (1)
46-46: Pin time=0.3.44 with correct features — LGTMVersion and features align with usage and current release. No issues.
Also applies to: 51-51
src/stdlib/time/mod.rs (6)
230-306: ISO-8601 duration formatting decomposition — LGTMFunction is within complexity bounds and handles edge cases (zero, negative, fractional). No issues.
214-228: UTC offset string formatting — LGTMCorrect sign handling and SS omission when zero; safe with i32 range of UtcOffset.
161-191: Data-driven timedelta accumulation — LGTMChecked arithmetic with clear overflow errors; CQRS and clippy-friendly. Solid improvement over repetitive code.
Also applies to: 185-195
197-212: Timestamp ISO-8601 with fractional-zero elision — LGTMInfallible fallback path is appropriate; zero-fraction trimming is safe and targeted.
328-341: Attribute exposure surface — LGTMAttributes match PR objectives (iso8601, unix_timestamp/offset, seconds/nanoseconds). Interface is minimal and predictable.
Also applies to: 365-378
39-50: Verify render-level determinism for now()
Guarantee that multiple calls tonow()within a single render return the same instant. If determinism is implemented elsewhere (e.g. timestamp captured once per render), add a brief comment referencing that; otherwise capture the clock once at the start of the render.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/stdlib/time/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Keep functions small with single responsibility; apply command-query separation
Function documentation must include clear examples; test documentation should omit redundant examples
No single Rust code file may exceed 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; code must be clean under clippy
Fix warnings emitted during tests in code, do not silence them
Extract helper functions when a function is too long; maintain separation of concerns and CQRS
Group many related parameters into well-named structs
Consider using Arc for large error values to reduce data returned
Every module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
When suppressing lints, scope narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint handling
When functions are unused for certain features, gate with #[cfg]/#[cfg_attr]
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use cap-std for filesystem operations; avoid std::fs directly
Use camino for path handling; avoid std::path::PathBuf directly
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use opaque eyre::Report only at the app boundary; do not expose in public APIs
Never export opaque error types from libraries; convert to domain...
Files:
src/stdlib/time/mod.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/stdlib/time/mod.rs
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-14, macos-arm64, arm64)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, x86_64)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64)
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (3)
src/stdlib/time/mod.rs (3)
52-59: Parse offset implementation is clean and correct.The function now uses
UtcOffset::parseas suggested in previous reviews, significantly reducing complexity whilst preserving the same error surface. The explicit handling of "Z" is clear and correct.Based on learnings
68-176: Data-driven approach eliminates repetition effectively.The use of
COMPONENT_SPECSwith a unifiedadd_componenthelper successfully addresses previous feedback about code duplication. TheComponentSpecstruct appropriately groups related parameters, and the iteration intimedeltais clean and maintainable.Based on learnings
211-272: Duration formatting is well-decomposed into focused helpers.The split into
format_duration_iso8601,format_time_components, andfinalize_duration_buffermaintains single responsibility and keeps complexity low. The implementation correctly handles ISO-8601 edge cases including zero durations, negative signs, and fractional seconds.
Extract a predicate for detecting timezone suffixes when trimming fractional seconds, tighten the offset parser to require an explicit sign before delegating to the time crate, and expand the expect message for fractional-second formatting.
1 participant
Summary
nowandtimedelta, including ISO-8601 formatting and offset parsingTesting
https://chatgpt.com/codex/tasks/task_e_68e2edccc8708322b3d8600dfb9c6c77
Summary by Sourcery
Add deterministic time template functions now() and timedelta() to the stdlib, with ISO-8601 formatting, offset parsing, documentation updates, and comprehensive tests
New Features:
Enhancements:
Build:
Documentation:
Tests: